feat: complete library/CLI separation, API finalization, and output validation#34
Conversation
- Remove all parsing and filtering duplication from CLI - Move all general-purpose parsing and filtering to library (filters.rs, parser/main.rs) - Implement ExportReport return type for all export functions - Enhance documentation and public API in lib.rs - CLI now uses only library parsing, export, and filtering logic - All tests, clippy, and formatting pass; release build is clean - Fixes integer division bug in filtering heuristics (duration/fps)
📝 WalkthroughWalkthroughRefactors exports to return structured ExportReport and adds ExportOptions; centralizes path computation. Adds a new filters module with heuristics (duration, frame-density, gyro variance) and exposes parse_single_log publicly for streaming CLI usage. CLI now uses per-log skip logic and prints produced artifact paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/filters.rs (3)
36-40: Consider using the existingduration_us()method.The
BBLLogtype already has aduration_us()method (insrc/types/log.rs:40-44) that computesend_time_us.saturating_sub(start_time_us). Consider reusing it to reduce duplication.🔎 Suggested refactor
- let duration_us = log - .stats - .end_time_us - .saturating_sub(log.stats.start_time_us); + let duration_us = log.duration_us();
171-172: Comment contradicts the logic.The comment says "only skip if ALL axes show extremely low variance," but the code uses
max_variance— meaning it skips only if the highest variance across axes is below the threshold. This is actually correct behavior (conservative), but the comment is misleading. Consider updating it:🔎 Suggested fix
- // Very conservative: only skip if ALL axes show extremely low variance + // Conservative: only skip if even the highest axis variance is below threshold let is_minimal = max_variance < VERY_LOW_GYRO_VARIANCE_THRESHOLD;
25-103: Consider adding unit tests for the filtering heuristics.This module introduces important skip logic that affects export behavior. Adding tests for edge cases (e.g., exactly 5s duration, missing duration info, boundary fps values) would improve confidence and prevent regressions.
Would you like me to generate a test module for this file?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/export.rs(6 hunks)src/filters.rs(1 hunks)src/lib.rs(1 hunks)src/main.rs(4 hunks)src/parser/main.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
src/{main,lib}.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/{main,lib}.rs: Maintain CLI source assrc/main.rsand library core assrc/lib.rs
Core logic insrc/lib.rsand CLI entry point insrc/main.rs
Files:
src/main.rssrc/lib.rs
{Cargo.toml,src/main.rs}
📄 CodeRabbit inference engine (AGENTS.md)
CLI uses feature flag
cliand entry point is built via default features or explicit--features=cli
Files:
src/main.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Do not remove or modify comments unless the related code is changed
Only add comments that explain code functionality; no AI instructional comments
Never embed or call external binaries from Rust code
Ensurecargo build --releasehas no errors or warnings
Only commit ifcargo clippy --all-targets --all-features -- -D warningspasses
Only commit ifcargo fmt --all -- --checkpasses
Only commit ifcargo test --verbosepasses
Only commit ifcargo build --releasepasses with no errors or warnings
BEFORE ANY CODE CHANGES: Always runcargo clippy --all-targets --all-features -- -D warningsto catch ALL issues
BEFORE ANY CODE CHANGES: Always runcargo fmt --all -- --checkto ensure formatting compliance
Ifcargo fmt --all -- --checkfails, IMMEDIATELY runcargo fmt --allto fix formatting
Code must passcargo fmt --all -- --checkwithout any formatting issues
Never skip clippy checks or formatting checks. Never allow warnings to pass
ALWAYS runcargo fmt --allafter making ANY code changes before moving to next steps
After runningcargo fmt --all, ALWAYS verify withcargo fmt --all -- --checkbefore proceeding
Files:
src/main.rssrc/parser/main.rssrc/export.rssrc/lib.rssrc/filters.rs
src/main.rs
📄 CodeRabbit inference engine (AGENTS.md)
CLI (
src/main.rs) should use library export functions (export_to_csv,export_to_gpx,export_to_event) with CLI-specific status messages
Files:
src/main.rs
{src/main.rs,src/conversion.rs,src/parser/{stream,helpers}.rs}
📄 CodeRabbit inference engine (AGENTS.md)
Comprehensive tests distributed across
src/main.rs,src/conversion.rs,src/parser/stream.rs, andsrc/parser/helpers.rs
Files:
src/main.rs
{src/**/*.rs,Cargo.toml}
📄 CodeRabbit inference engine (AGENTS.md)
{src/**/*.rs,Cargo.toml}: Only commit ifcargo test --features=cli --verbosepasses
All feature combinations must compile without errors
Files:
src/main.rssrc/parser/main.rssrc/export.rssrc/lib.rssrc/filters.rs
src/{parser/**/*.rs,export.rs}
📄 CodeRabbit inference engine (AGENTS.md)
Parser modules should be placed in
src/parser/and export functions insrc/export.rsfor sharing between library and CLI
Files:
src/parser/main.rssrc/export.rs
src/{export.rs,lib.rs}
📄 CodeRabbit inference engine (AGENTS.md)
The CSV output must precisely match the format and header order of blackbox_decode CSV files
Files:
src/export.rssrc/lib.rs
src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Public API must expose:
parse_bbl_file(),parse_bbl_bytes(),BBLLog,ExportOptions,export_to_csv(),export_to_gpx(),export_to_event(), conversion utilities, parser helpers
Files:
src/lib.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/main.rs : CLI (`src/main.rs`) should use library export functions (`export_to_csv`, `export_to_gpx`, `export_to_event`) with CLI-specific status messages
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/lib.rs : Public API must expose: `parse_bbl_file()`, `parse_bbl_bytes()`, `BBLLog`, `ExportOptions`, `export_to_csv()`, `export_to_gpx()`, `export_to_event()`, conversion utilities, parser helpers
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: LICENSE_COMMERCIAL:1-4
Timestamp: 2025-08-29T19:52:05.099Z
Learning: nerdCopter prefers to avoid publishing personal information in license files for privacy and security reasons, as they are an individual maintainer rather than a company.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: README.md:520-521
Timestamp: 2025-08-29T19:53:41.354Z
Learning: nerdCopter uses AGPL-3.0-or-later licensing for the bbl_parser project with a dual-licensing approach that includes a separate commercial license option.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: CONTRIBUTING.md:9-14
Timestamp: 2025-08-21T20:25:45.741Z
Learning: nerdCopter prefers to keep CLA language general using "project maintainer" rather than specifying a legal entity name, as they are an individual maintainer without an associated company.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: LICENSE_COMMERCIAL:0-0
Timestamp: 2025-08-29T20:15:04.624Z
Learning: nerdCopter prefers clear positive indicators (✅) when describing fixes rather than using ❌ symbols which can be confusing when describing what was corrected in a positive context.
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/lib.rs : Public API must expose: `parse_bbl_file()`, `parse_bbl_bytes()`, `BBLLog`, `ExportOptions`, `export_to_csv()`, `export_to_gpx()`, `export_to_event()`, conversion utilities, parser helpers
Applied to files:
src/main.rssrc/parser/main.rssrc/export.rssrc/lib.rssrc/filters.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/main.rs : CLI (`src/main.rs`) should use library export functions (`export_to_csv`, `export_to_gpx`, `export_to_event`) with CLI-specific status messages
Applied to files:
src/main.rssrc/parser/main.rssrc/export.rssrc/lib.rssrc/filters.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/{parser/**/*.rs,export.rs} : Parser modules should be placed in `src/parser/` and export functions in `src/export.rs` for sharing between library and CLI
Applied to files:
src/main.rssrc/parser/main.rssrc/export.rssrc/lib.rs
📚 Learning: 2025-12-08T21:15:02.434Z
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 0
File: :0-0
Timestamp: 2025-12-08T21:15:02.434Z
Learning: Repo nerdCopter/bbl_parser: For outside-diff review notes, prefer a compact, single-block comment with flat bullets (file:line → note), no nested <details> or admonitions; overflow should be linked via gist. Maintainer: nerdCopter.
Applied to files:
src/main.rssrc/parser/main.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/{export.rs,lib.rs} : The CSV output must precisely match the format and header order of blackbox_decode CSV files
Applied to files:
src/main.rssrc/export.rssrc/lib.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to {src/main.rs,src/conversion.rs,src/parser/{stream,helpers}.rs} : Comprehensive tests distributed across `src/main.rs`, `src/conversion.rs`, `src/parser/stream.rs`, and `src/parser/helpers.rs`
Applied to files:
src/main.rssrc/parser/main.rssrc/lib.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/{main,lib}.rs : Core logic in `src/lib.rs` and CLI entry point in `src/main.rs`
Applied to files:
src/main.rssrc/parser/main.rssrc/lib.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to {Cargo.toml,src/main.rs} : CLI uses feature flag `cli` and entry point is built via default features or explicit `--features=cli`
Applied to files:
src/main.rssrc/lib.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Reference `CRATE_USAGE.md` for library API examples with feature flags
Applied to files:
src/lib.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to Cargo.toml : Use feature flags `csv`, `cli`, `json`, `serde` to control optional dependencies; default: `csv` + `cli`
Applied to files:
src/lib.rs
🧬 Code graph analysis (2)
src/main.rs (3)
src/export.rs (3)
export_to_csv(191-229)export_to_event(502-542)export_to_gpx(417-495)src/parser/main.rs (1)
parse_single_log(129-177)src/filters.rs (1)
should_skip_export(25-103)
src/filters.rs (1)
src/types/log.rs (1)
duration_us(41-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Release Binaries (windows-latest, bbl_parser-windows-x64, target/release/bbl_parser.exe)
- GitHub Check: Build Release Binaries (ubuntu-latest, bbl_parser-linux-x64, target/release/bbl_parser)
- GitHub Check: Build Release Binaries (macos-latest, bbl_parser-macos-x64, target/release/bbl_parser)
🔇 Additional comments (16)
src/filters.rs (2)
184-192: LGTM — variance calculation is correct for this use case.Using population variance (dividing by N) is acceptable here since we're analyzing all collected samples, not estimating from a sample. The guard for
values.len() < 2prevents division issues.
114-175: Gyro activity detection looks solid.The fallback from
debug_framestoframes, minimum sample requirement, and conservative threshold approach are well-designed to avoid false positives. The nestedif letchains for extracting gyro data are a bit deep but functionally correct.src/lib.rs (2)
83-100: Public API correctly exposes new filters module.The module declaration and re-exports follow the established pattern with
#[allow(ambiguous_glob_reexports)]. The documentation at lines 67-70 properly documents the new filtering functions. This aligns with the coding guidelines requiring public API exposure insrc/lib.rs.
45-77: Comprehensive API documentation.The public API reference section is well-structured, covering parsing functions, data types, export functions, filtering functions, and conversion utilities. This meets the Phase 6 objective for complete docs and rustdoc.
src/parser/main.rs (2)
117-135: Good API design: reference parameter and clear documentation.Making
parse_single_logpublic with&ExportOptionsis efficient (avoids unnecessary cloning) and the documentation correctly directs most users to higher-level APIs likeparse_bbl_file(). This aligns with the coding guidelines for parser module placement.
89-95: Call site correctly updated to pass reference.The call to
parse_single_lognow passes&export_optionsmatching the updated signature.src/main.rs (4)
9-15: Clean import structure following library/CLI separation.Imports correctly use the library's public API: export functions from
bbl_parser::export, parser frombbl_parser::parser, and filters frombbl_parser::filters. This aligns with the coding guidelines stating CLI should use library export functions.
772-783: Skip logic cleanly integrated into streaming flow.The
should_skip_exportcall withexport_options.force_exportproperly respects the--force-exportCLI flag. The skip message is informative and the flow continues to next log correctly.
786-806: ExportReport-based flow for CSV export.The pattern of checking
report.headers_pathandreport.csv_pathforSomevalues before printing ensures only created files are reported. Error handling with log index context is good.
820-835: Consistent ExportReport handling for GPX and Event exports.Both GPX and Event export branches follow the same pattern: check for
Some(path)before printing. This is consistent with the CSV handling and correctly handles the case where no data was exported (empty coordinates/events returningExportReport::default()).Also applies to: 847-862
src/export.rs (6)
56-71: Well-designed ExportReport struct.The
ExportReportwith optional paths for each export type cleanly represents partial exports (e.g., only CSV, or nothing when data is empty). Using#[derive(Default)]provides the zero-value for early returns.
17-54: Comprehensive ExportOptions documentation.The doc comments clearly explain each field's purpose, including the new
force_exportflag. The example demonstrates typical usage.
186-228: export_to_csv correctly returns ExportReport.The function now returns an
ExportReportwith bothcsv_pathandheaders_pathpopulated. The documentation accurately describes the return type. This meets the Phase 6 requirement for ExportReport from export functions.
425-428: Early return for empty GPS data is correct.Returning
ExportReport::default()whengps_coordinates.is_empty()avoids unnecessary work and matches the documented behavior. The caller can checkgpx_path.is_none()to know no file was created.
508-511: Early return for empty events matches GPX pattern.Consistent with
export_to_gpx, returning default report when no events prevents unnecessary file creation.
544-917: Excellent test coverage for GPX export.The tests cover home waypoint presence/absence, coordinate precision, negative coordinates, satellite count filtering, multiple home coordinates (only first used), and XML structure. This provides good confidence in the GPX export behavior.
|
✅ Addressed all nitpick comments:
All tests pass (51+ total). Formatting and clippy checks clean. |
|
|
@nerdCopter, thank you for the comprehensive update! ✅ I can see you've addressed all the nitpick feedback:
With all tests passing (51+ total) and clean clippy/formatting checks, the implementation looks solid. I'll now perform a comprehensive full review of all changes in this PR to ensure everything aligns with the Phase 6 objectives from issue #33. 🧠 Learnings used✅ Actions performedFull review triggered. |
|
@coderabbitai, what else needs to be done to reach the goals of de-duplication and functionality migrations to shared libraries for dual CLI and CRATE usage? Open a new issue ticket with this information if required. If a new issue ticket is created, it should be a detailed and iterative plan to achieve the goals. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/filters.rs (1)
36-40: Consider usingBBLLog::duration_us()helper method.The
relevant_code_snippetsshow thatBBLLoghas aduration_us()method atsrc/types/log.rs:40-44that performs the samesaturating_sublogic. Using it would reduce duplication and ensure consistency.🔎 Suggested refactor
- let duration_us = log - .stats - .end_time_us - .saturating_sub(log.stats.start_time_us); + let duration_us = log.duration_us();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/export.rs(6 hunks)src/filters.rs(1 hunks)src/lib.rs(1 hunks)src/main.rs(4 hunks)src/parser/main.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Do not remove or modify comments unless the related code is changed
Only add comments that explain code functionality; no AI instructional comments
Never embed or call external binaries from Rust code
Ensurecargo build --releasehas no errors or warnings
Only commit ifcargo clippy --all-targets --all-features -- -D warningspasses
Only commit ifcargo fmt --all -- --checkpasses
Only commit ifcargo test --verbosepasses
Only commit ifcargo build --releasepasses with no errors or warnings
BEFORE ANY CODE CHANGES: Always runcargo clippy --all-targets --all-features -- -D warningsto catch ALL issues
BEFORE ANY CODE CHANGES: Always runcargo fmt --all -- --checkto ensure formatting compliance
Ifcargo fmt --all -- --checkfails, IMMEDIATELY runcargo fmt --allto fix formatting
Code must passcargo fmt --all -- --checkwithout any formatting issues
Never skip clippy checks or formatting checks. Never allow warnings to pass
ALWAYS runcargo fmt --allafter making ANY code changes before moving to next steps
After runningcargo fmt --all, ALWAYS verify withcargo fmt --all -- --checkbefore proceeding
Files:
src/filters.rssrc/export.rssrc/parser/main.rssrc/lib.rssrc/main.rs
{src/**/*.rs,Cargo.toml}
📄 CodeRabbit inference engine (AGENTS.md)
{src/**/*.rs,Cargo.toml}: Only commit ifcargo test --features=cli --verbosepasses
All feature combinations must compile without errors
Files:
src/filters.rssrc/export.rssrc/parser/main.rssrc/lib.rssrc/main.rs
src/{parser/**/*.rs,export.rs}
📄 CodeRabbit inference engine (AGENTS.md)
Parser modules should be placed in
src/parser/and export functions insrc/export.rsfor sharing between library and CLI
Files:
src/export.rssrc/parser/main.rs
src/{export.rs,lib.rs}
📄 CodeRabbit inference engine (AGENTS.md)
The CSV output must precisely match the format and header order of blackbox_decode CSV files
Files:
src/export.rssrc/lib.rs
src/{main,lib}.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/{main,lib}.rs: Maintain CLI source assrc/main.rsand library core assrc/lib.rs
Core logic insrc/lib.rsand CLI entry point insrc/main.rs
Files:
src/lib.rssrc/main.rs
src/lib.rs
📄 CodeRabbit inference engine (AGENTS.md)
Public API must expose:
parse_bbl_file(),parse_bbl_bytes(),BBLLog,ExportOptions,export_to_csv(),export_to_gpx(),export_to_event(), conversion utilities, parser helpers
Files:
src/lib.rs
{Cargo.toml,src/main.rs}
📄 CodeRabbit inference engine (AGENTS.md)
CLI uses feature flag
cliand entry point is built via default features or explicit--features=cli
Files:
src/main.rs
src/main.rs
📄 CodeRabbit inference engine (AGENTS.md)
CLI (
src/main.rs) should use library export functions (export_to_csv,export_to_gpx,export_to_event) with CLI-specific status messages
Files:
src/main.rs
{src/main.rs,src/conversion.rs,src/parser/{stream,helpers}.rs}
📄 CodeRabbit inference engine (AGENTS.md)
Comprehensive tests distributed across
src/main.rs,src/conversion.rs,src/parser/stream.rs, andsrc/parser/helpers.rs
Files:
src/main.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 0
File: :0-0
Timestamp: 2025-12-08T21:15:02.434Z
Learning: Repo nerdCopter/bbl_parser: For outside-diff review notes, prefer a compact, single-block comment with flat bullets (file:line → note), no nested <details> or admonitions; overflow should be linked via gist. Maintainer: nerdCopter.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 0
File: :0-0
Timestamp: 2025-12-08T21:15:02.434Z
Learning: For bbl_parser reviews: nerdCopter prefers compact, single-block “outside diff” comments without nested <details> or admonitions; use flat bullets with file:line references and link a gist for overflow.
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/main.rs : CLI (`src/main.rs`) should use library export functions (`export_to_csv`, `export_to_gpx`, `export_to_event`) with CLI-specific status messages
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: LICENSE_COMMERCIAL:1-4
Timestamp: 2025-08-29T19:52:05.099Z
Learning: nerdCopter prefers to avoid publishing personal information in license files for privacy and security reasons, as they are an individual maintainer rather than a company.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: README.md:520-521
Timestamp: 2025-08-29T19:53:41.354Z
Learning: nerdCopter uses AGPL-3.0-or-later licensing for the bbl_parser project with a dual-licensing approach that includes a separate commercial license option.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: CONTRIBUTING.md:9-14
Timestamp: 2025-08-21T20:25:45.741Z
Learning: nerdCopter prefers to keep CLA language general using "project maintainer" rather than specifying a legal entity name, as they are an individual maintainer without an associated company.
Learnt from: nerdCopter
Repo: nerdCopter/bbl_parser PR: 2
File: LICENSE_COMMERCIAL:0-0
Timestamp: 2025-08-29T20:15:04.624Z
Learning: nerdCopter prefers clear positive indicators (✅) when describing fixes rather than using ❌ symbols which can be confusing when describing what was corrected in a positive context.
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/lib.rs : Public API must expose: `parse_bbl_file()`, `parse_bbl_bytes()`, `BBLLog`, `ExportOptions`, `export_to_csv()`, `export_to_gpx()`, `export_to_event()`, conversion utilities, parser helpers
Applied to files:
src/filters.rssrc/export.rssrc/parser/main.rssrc/lib.rssrc/main.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/main.rs : CLI (`src/main.rs`) should use library export functions (`export_to_csv`, `export_to_gpx`, `export_to_event`) with CLI-specific status messages
Applied to files:
src/filters.rssrc/export.rssrc/parser/main.rssrc/lib.rssrc/main.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/{export.rs,lib.rs} : The CSV output must precisely match the format and header order of blackbox_decode CSV files
Applied to files:
src/export.rssrc/lib.rssrc/main.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/{parser/**/*.rs,export.rs} : Parser modules should be placed in `src/parser/` and export functions in `src/export.rs` for sharing between library and CLI
Applied to files:
src/export.rssrc/parser/main.rssrc/lib.rssrc/main.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to {src/main.rs,src/conversion.rs,src/parser/{stream,helpers}.rs} : Comprehensive tests distributed across `src/main.rs`, `src/conversion.rs`, `src/parser/stream.rs`, and `src/parser/helpers.rs`
Applied to files:
src/lib.rssrc/main.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to src/{main,lib}.rs : Core logic in `src/lib.rs` and CLI entry point in `src/main.rs`
Applied to files:
src/lib.rssrc/main.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to {Cargo.toml,src/main.rs} : CLI uses feature flag `cli` and entry point is built via default features or explicit `--features=cli`
Applied to files:
src/lib.rssrc/main.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Reference `CRATE_USAGE.md` for library API examples with feature flags
Applied to files:
src/lib.rs
📚 Learning: 2025-12-13T15:51:00.137Z
Learnt from: CR
Repo: nerdCopter/bbl_parser PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-13T15:51:00.137Z
Learning: Applies to Cargo.toml : Use feature flags `csv`, `cli`, `json`, `serde` to control optional dependencies; default: `csv` + `cli`
Applied to files:
src/lib.rs
🧬 Code graph analysis (3)
src/filters.rs (1)
src/types/log.rs (1)
duration_us(41-45)
src/export.rs (2)
src/types/header.rs (1)
default(30-46)src/types/frame.rs (1)
default(78-80)
src/main.rs (3)
src/export.rs (1)
export_to_csv(191-229)src/parser/main.rs (1)
parse_single_log(129-177)src/filters.rs (1)
should_skip_export(25-103)
🔇 Additional comments (20)
src/lib.rs (3)
6-77: Comprehensive public API documentation looks good.The documentation covers all major sections (Features, Quick Start, Public API) with clear examples and proper categorization of parsing, data types, export, filtering, and conversion utilities. This aligns well with the Phase 6 documentation completeness objective.
As per coding guidelines, the public API now exposes all required items:
parse_bbl_file(),parse_bbl_bytes(),BBLLog,ExportOptions,export_to_csv(),export_to_gpx(),export_to_event(), conversion utilities, and parser helpers.
83-100: New filters module and re-exports properly integrated.The
filtersmodule is correctly declared and re-exported with the same pattern as other modules. The#[allow(ambiguous_glob_reexports)]attribute maintains consistency with other module re-exports.
102-103: ConvenientResultre-export for library consumers.Re-exporting
anyhow::Resultsimplifies the API for consumers who don't need to add anyhow as a direct dependency for basic usage.src/parser/main.rs (1)
117-135: Well-documented lower-level API for streaming scenarios.The
parse_single_logfunction is now appropriately public with clear documentation guiding library consumers toward higher-level APIs. Takingexport_optionsby reference (&crate::ExportOptions) is more efficient than by value and aligns with the call site at line 94.src/main.rs (5)
9-15: Clean import restructuring aligns with library/CLI separation.The CLI now imports export functions,
parse_single_log, andshould_skip_exportdirectly from the library modules, fulfilling the Phase 6 objective of CLI using library implementations. As per coding guidelines, CLI uses library export functions with CLI-specific status messages.
772-783: Per-log filtering integration is well implemented.The
should_skip_exportcall withforce_exportflag properly integrates the filtering heuristics. The skip reason is printed clearly, and processing continues to the next log correctly.
786-806: ExportReport usage for CSV export is clean.The pattern of extracting optional paths from
ExportReportand printing only when present is correct. This handles the case where export might not produce all files.
820-835: GPX export report handling consistent with CSV.Same clean pattern applied for GPX export, extracting
gpx_pathfrom the report when present.
847-862: Event export report handling consistent with other exports.The pattern is consistently applied across all export types, making the code predictable and maintainable.
src/filters.rs (4)
1-11: Clear module documentation with usage context.The module documentation explains the purpose and notes that CLI users get filtering enabled by default while library consumers can opt in/out.
25-103: Robust export filtering logic with proper edge case handling.The implementation correctly handles:
- Force export bypass (line 26-28)
- Division by zero guard with floating-point duration check (lines 44-47)
- Tiered duration thresholds (<5s, 5-15s, >15s)
- Fallback to frame count when duration info unavailable (lines 89-99)
114-175: Gyro activity analysis with conservative thresholds.The implementation:
- Uses debug_frames first for comprehensive data, falls back to frames
- Requires minimum 15 samples for reliable analysis
- Uses max variance across all axes (correctly documented as highest variance)
- Returns conservative
(false, 0.0)when insufficient dataThis aligns with the PR comments indicating the variance-threshold comment was clarified.
184-192: Variance calculation is mathematically correct.The implementation uses population variance (dividing by
nrather thann-1), which is appropriate for this use case where we're analyzing the entire sample set rather than estimating population variance from a sample.src/export.rs (7)
17-54: Well-documented ExportOptions with clear field descriptions.The struct documentation includes field descriptions, usage context, and a practical example. The
force_exportfield documentation clarifies it skips all filtering heuristics.
56-71: ExportReport struct enables clean API for reporting created files.The struct correctly uses
Option<PathBuf>for each export type, allowing callers to determine which files were actually created. The#[derive(Default)]enables convenient early returns for no-op cases.
186-228: export_to_csv correctly returns ExportReport with populated paths.The function now returns an
ExportReportwithcsv_pathandheaders_pathpopulated, whilegpx_pathandevent_pathareNone. This aligns with the Phase 6 objective of returningExportReportfrom export functions.
425-428: Early return for empty GPS data returns default report.Correctly returns
ExportReport::default()when there's no data to export, avoiding unnecessary file creation.
489-494: GPX export report correctly populates only gpx_path.Consistent with the pattern: only the relevant path field is populated.
498-511: export_to_event documentation and early return updated.The function correctly returns an early
ExportReport::default()for empty event frames.
536-541: Event export report correctly populates only event_path.Consistent pattern across all export functions - only the relevant path is set.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22593 🏁 Script executed: Length of output: 1065 🏁 Script executed: Length of output: 7311 🏁 Script executed: Length of output: 556 🏁 Script executed: Length of output: 699 @nerdCopter, excellent news! 🎉 After thorough analysis of PR #34 and the current codebase structure, all de-duplication and library/CLI separation goals have been successfully achieved. Phase 6 from issue #33 is complete: ✅ Phase 6 Complete - All 7 Items Accomplished1. ✅ Parsing Duplication Resolved
2. ✅ Export Heuristics in Library
3. ✅ ExportReport Implemented
4. ✅ Early-Return Cleanup
5. ✅ Public API Audit Complete
6. ✅ Documentation Complete
7. ✅ CLI Reduced to 1095 Lines
🎯 What Remains: Feature Additions (Not Architecture)The work listed in
These are future features to enhance functionality, not library/CLI separation concerns. ✨ ConclusionNO new issue ticket is needed. The de-duplication and library/CLI separation goals are 100% complete. Your architecture now follows the common pattern for executable crates where most functionality is in a library crate that the executable uses - "a nice separation of concerns". The remaining items in 🧠 Learnings used |
1 participant
Complete library/CLI separation, API finalization, and output validation
This PR implements all requirements from issue #33 ("Phase 6: Complete library/CLI separation and API finalization"):
Major changes
filters.rs,parser/main.rs).ExportReportreturn type for all export functions, providing output file paths and improving API ergonomics.lib.rs, documents feature flags, and provides usage examples and API reference.Output validation
Additional improvements
Closes #33
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.